Pushdown sort aggregate metrics by LantaoJin · Pull Request #4603 · opensearch-project/sql

LantaoJin · 2025-10-20T09:16:34Z

Description

Pushdown sort aggregate metrics:

Add SORT_AGG_METRICS after AGGREGATION in pushdown context
Convert composite bucket agg to terms/histo/date-histo agg if sort one metric on single bucket
Convert composite bucket agg (all terms source) to multi-terms agg if sort one metric on multiple buckets

Related Issues

Resolves #4282

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
New PPL command checklist all confirmed.
API changes companion pull request created.
Commits are signed per the DCO using --signoff or -s.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Lantao Jin <ltjin@amazon.com>

...src/main/java/org/opensearch/sql/opensearch/planner/physical/SortAggregationMetricsRule.java

...arch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggPushDownAction.java

qianheng-aws · 2025-10-23T08:47:01Z

...search/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java

    return newTraitSet;
  }

+  public CalciteLogicalIndexScan pushDownSortAggregateMetrics(Sort sort) {


We can only do this operation when bucket_nullable=false, right? Any place to check this parameter?

AggPushDownAction.L97 and L126

[nit] Will it be better to put these checks in one place if possible? Now the prerequisites are split both here and in aggAction.pushDownSortAggMetrics, although they are both aggregationBuilders and some checking seems overlapped.

Cannot get the bucket_nullable information here. Aggregation has already pushed down. So AggPushDownAction.L97 and L126 would be better IMO.

But I see you've already get the pushed aggregation builder here.

List<AggregationBuilder> aggregationBuilders = pushDownContext.getAggPushDownAction().getAggregationBuilder().getLeft();

Or otherwise L290-310 could all be moved into aggAction.pushDownSortAggMetrics.

It's also fine to me if not making change since the logic is correct anyway.

I see, but I don't want always to check CompositeValuesSourceBuilder.missingBucket() here since not all ValuesSourceBuilders are impacted by bucket_nullable, for example date histogram. So moving to here has to check types, sounds not worth to check types twice.

Signed-off-by: Lantao Jin <ltjin@amazon.com>

qianheng-aws · 2025-10-24T03:10:14Z

...search/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java

    return newTraitSet;
  }

+  public CalciteLogicalIndexScan pushDownSortAggregateMetrics(Sort sort) {


[nit] Will it be better to put these checks in one place if possible? Now the prerequisites are split both here and in aggAction.pushDownSortAggMetrics, although they are both aggregationBuilders and some checking seems overlapped.

...arch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggPushDownAction.java

… buckets Signed-off-by: Lantao Jin <ltjin@amazon.com>

qianheng-aws · 2025-10-24T06:34:11Z

...arch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggPushDownAction.java

+      // count agg optimized, get the path name from field names
+      path = fieldNames.get(collations.get(0).getFieldIndex());
+    } else if (metric instanceof ValuesSourceAggregationBuilder.LeafOnly) {
+      path = metric.getName();


[nit, non-blocking] Could this be combined with above branch? i.e. Does path = fieldNames.get(collations.get(0).getFieldIndex()); also works for metric instanceof ValuesSourceAggregationBuilder.LeafOnly?

qianheng-aws · 2025-10-24T06:43:05Z

...arch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggPushDownAction.java

+      List<String> fieldNames,
+      CompositeAggregationBuilder composite) {
+    String path;
+    AggregationBuilder metric = composite.getSubAggregations().stream().findFirst().orElse(null);


Do we only support only 1 agg metric for this enhancement? And anywhere to check this?

Do we only support only 1 agg metric for this enhancement? And anywhere to check this?

https://github.com/opensearch-project/sql/pull/4603/files#diff-b4a250f8c0175f824789fb6cd403e69f4de986c912461daf766d39442a7f530bR40

good catching. added restricted checking.

Signed-off-by: Lantao Jin <ltjin@amazon.com>

opensearch-trigger-bot · 2025-10-24T09:04:01Z

The backport to 2.19-dev failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.19-dev 2.19-dev
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.19-dev
# Create a new branch
git switch --create backport/backport-4603-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5ebed8465dc9d498602a25b802207d63ee5030c8
# Push it to GitHub
git push --set-upstream origin backport/backport-4603-to-2.19-dev
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.19-dev

Then, create a pull request where the base branch is 2.19-dev and the compare/head branch is backport/backport-4603-to-2.19-dev.

* convert sort aggregate metrics to term sort Signed-off-by: Lantao Jin <ltjin@amazon.com> * refactor Signed-off-by: Lantao Jin <ltjin@amazon.com> * fix conflicts Signed-off-by: Lantao Jin <ltjin@amazon.com> * fix conflicts2 Signed-off-by: Lantao Jin <ltjin@amazon.com> * Add more javadoc Signed-off-by: Lantao Jin <ltjin@amazon.com> * address comments Signed-off-by: Lantao Jin <ltjin@amazon.com> * delete incorrect comments Signed-off-by: Lantao Jin <ltjin@amazon.com> * convert composite agg to multi-terms agg for sort metrics on multiple buckets Signed-off-by: Lantao Jin <ltjin@amazon.com> * avoid the case of 'stat count, sum ... | sort count' Signed-off-by: Lantao Jin <ltjin@amazon.com> --------- Signed-off-by: Lantao Jin <ltjin@amazon.com> (cherry picked from commit 5ebed84)

* Pushdown sort aggregate metrics (#4603) * convert sort aggregate metrics to term sort Signed-off-by: Lantao Jin <ltjin@amazon.com> * refactor Signed-off-by: Lantao Jin <ltjin@amazon.com> * fix conflicts Signed-off-by: Lantao Jin <ltjin@amazon.com> * fix conflicts2 Signed-off-by: Lantao Jin <ltjin@amazon.com> * Add more javadoc Signed-off-by: Lantao Jin <ltjin@amazon.com> * address comments Signed-off-by: Lantao Jin <ltjin@amazon.com> * delete incorrect comments Signed-off-by: Lantao Jin <ltjin@amazon.com> * convert composite agg to multi-terms agg for sort metrics on multiple buckets Signed-off-by: Lantao Jin <ltjin@amazon.com> * avoid the case of 'stat count, sum ... | sort count' Signed-off-by: Lantao Jin <ltjin@amazon.com> --------- Signed-off-by: Lantao Jin <ltjin@amazon.com> (cherry picked from commit 5ebed84) * fix conflicts Signed-off-by: Lantao Jin <ltjin@amazon.com> --------- Signed-off-by: Lantao Jin <ltjin@amazon.com>

* default-main: (34 commits) Enhance dynamic source clause to support only metadata filters (opensearch-project#4554) Make nested alias type support referring to outer context (opensearch-project#4673) Update big5 ppl queries and check plans (opensearch-project#4668) Support push down sort after limit (opensearch-project#4657) Use table scan rowType in filter pushdown could fix rename issue (opensearch-project#4670) Fix: Support Alias Fields in MIN, MAX, FIRST, LAST, and TAKE Aggregations (opensearch-project#4621) Fix bin nested fields issue (opensearch-project#4606) Add `per_minute`, `per_hour`, `per_day` function support (opensearch-project#4531) Pushdown sort aggregate metrics (opensearch-project#4603) Followup: Change ComparableLinkedHashMap to compare Key than Value (opensearch-project#4648) Mitigate the CI failure caused by 500 Internal Server Error (opensearch-project#4646) Allow renaming group-by fields to existing field names (opensearch-project#4586) Publish internal modules separately for downstream reuse (opensearch-project#4484) Revert "Update grammar files and developer guide (opensearch-project#4301)" (opensearch-project#4643) Support Automatic Type Conversion for REX/SPATH/PARSE Command Extractions (opensearch-project#4599) Replace all dots in fields of table scan's PhysType (opensearch-project#4633) Return comparable LinkedHashMap in `valueForCalcite()` of ExprTupleValue (opensearch-project#4629) Refactor JsonExtractAllFunctionIT and MapConcatFunctionIT (opensearch-project#4623) Pushdown case function in aggregations as range queries (opensearch-project#4400) Update GEOIP function to support IP types as input (opensearch-project#4613) ... # Conflicts: # docs/user/ppl/functions/conversion.rst

* default-main: (34 commits) Enhance dynamic source clause to support only metadata filters (opensearch-project#4554) Make nested alias type support referring to outer context (opensearch-project#4673) Update big5 ppl queries and check plans (opensearch-project#4668) Support push down sort after limit (opensearch-project#4657) Use table scan rowType in filter pushdown could fix rename issue (opensearch-project#4670) Fix: Support Alias Fields in MIN, MAX, FIRST, LAST, and TAKE Aggregations (opensearch-project#4621) Fix bin nested fields issue (opensearch-project#4606) Add `per_minute`, `per_hour`, `per_day` function support (opensearch-project#4531) Pushdown sort aggregate metrics (opensearch-project#4603) Followup: Change ComparableLinkedHashMap to compare Key than Value (opensearch-project#4648) Mitigate the CI failure caused by 500 Internal Server Error (opensearch-project#4646) Allow renaming group-by fields to existing field names (opensearch-project#4586) Publish internal modules separately for downstream reuse (opensearch-project#4484) Revert "Update grammar files and developer guide (opensearch-project#4301)" (opensearch-project#4643) Support Automatic Type Conversion for REX/SPATH/PARSE Command Extractions (opensearch-project#4599) Replace all dots in fields of table scan's PhysType (opensearch-project#4633) Return comparable LinkedHashMap in `valueForCalcite()` of ExprTupleValue (opensearch-project#4629) Refactor JsonExtractAllFunctionIT and MapConcatFunctionIT (opensearch-project#4623) Pushdown case function in aggregations as range queries (opensearch-project#4400) Update GEOIP function to support IP types as input (opensearch-project#4613) ... Signed-off-by: Asif Bashar <asif.bashar@gmail.com>

* convert sort aggregate metrics to term sort Signed-off-by: Lantao Jin <ltjin@amazon.com> * refactor Signed-off-by: Lantao Jin <ltjin@amazon.com> * fix conflicts Signed-off-by: Lantao Jin <ltjin@amazon.com> * fix conflicts2 Signed-off-by: Lantao Jin <ltjin@amazon.com> * Add more javadoc Signed-off-by: Lantao Jin <ltjin@amazon.com> * address comments Signed-off-by: Lantao Jin <ltjin@amazon.com> * delete incorrect comments Signed-off-by: Lantao Jin <ltjin@amazon.com> * convert composite agg to multi-terms agg for sort metrics on multiple buckets Signed-off-by: Lantao Jin <ltjin@amazon.com> * avoid the case of 'stat count, sum ... | sort count' Signed-off-by: Lantao Jin <ltjin@amazon.com> --------- Signed-off-by: Lantao Jin <ltjin@amazon.com>

LantaoJin added 2 commits October 20, 2025 17:09

convert sort aggregate metrics to term sort

7ddb1d7

Signed-off-by: Lantao Jin <ltjin@amazon.com>

Merge remote-tracking branch 'upstream/main' into issues/4282

07ddaa3

Signed-off-by: Lantao Jin <ltjin@amazon.com>

LantaoJin added the enhancement New feature or request label Oct 21, 2025

LantaoJin added 2 commits October 21, 2025 18:26

refactor

a8a7553

Signed-off-by: Lantao Jin <ltjin@amazon.com>

Merge remote-tracking branch 'upstream/main' into issues/4282

eb4627f

Signed-off-by: Lantao Jin <ltjin@amazon.com>

LantaoJin marked this pull request as ready for review October 21, 2025 10:36

LantaoJin added 2 commits October 21, 2025 18:37

fix conflicts

4cf940d

Signed-off-by: Lantao Jin <ltjin@amazon.com>

fix conflicts2

1a1fe05

Signed-off-by: Lantao Jin <ltjin@amazon.com>

qianheng-aws reviewed Oct 23, 2025

View reviewed changes

LantaoJin added 2 commits October 23, 2025 21:52

address comments

0a6bc4f

Signed-off-by: Lantao Jin <ltjin@amazon.com>

Merge remote-tracking branch 'upstream/main' into issues/4282

2582955

Signed-off-by: Lantao Jin <ltjin@amazon.com>

LantaoJin requested review from qianheng-aws and yuancu October 23, 2025 14:02

delete incorrect comments

e77ab8e

Signed-off-by: Lantao Jin <ltjin@amazon.com>

LantaoJin changed the title ~~Pushdown sort aggregate metrics~~ Pushdown sort aggregate metrics for single bucket Oct 24, 2025

qianheng-aws reviewed Oct 24, 2025

View reviewed changes

LantaoJin added 2 commits October 24, 2025 12:59

convert composite agg to multi-terms agg for sort metrics on multiple…

a7a1501

… buckets Signed-off-by: Lantao Jin <ltjin@amazon.com>

Merge remote-tracking branch 'upstream/main' into issues/4282

37ab135

LantaoJin changed the title ~~Pushdown sort aggregate metrics for single bucket~~ Pushdown sort aggregate metrics Oct 24, 2025

qianheng-aws reviewed Oct 24, 2025

View reviewed changes

avoid the case of 'stat count, sum ... | sort count'

416ca6d

Signed-off-by: Lantao Jin <ltjin@amazon.com>

LantaoJin enabled auto-merge (squash) October 24, 2025 08:47

LantaoJin added the backport 2.19-dev label Oct 24, 2025

qianheng-aws approved these changes Oct 24, 2025

View reviewed changes

yuancu approved these changes Oct 24, 2025

View reviewed changes

LantaoJin merged commit 5ebed84 into opensearch-project:main Oct 24, 2025
35 checks passed

opensearch-trigger-bot bot added the backport-failed label Oct 24, 2025

LantaoJin mentioned this pull request Oct 24, 2025

[Backport 2.19-dev] Pushdown sort aggregate metrics (#4603) #4658

Merged

LantaoJin added the backport-manually Filed a PR to backport manually. label Oct 24, 2025

LantaoJin mentioned this pull request Oct 27, 2025

[BUG] Sorting rules not applied to bucket aggregations #4278

Open

LantaoJin mentioned this pull request Oct 30, 2025

[BUG] Cannot parse the bucket if we optimize a composite to non-composite aggregate #4705

Closed

qianheng-aws mentioned this pull request Nov 4, 2025

[FEATURE] Support push down sort on metrics for aggregation with more than one agg call #4737

Closed

Conversation

LantaoJin commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LantaoJin Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

opensearch-trigger-bot bot commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LantaoJin commented Oct 20, 2025 •

edited

Loading

LantaoJin Oct 24, 2025 •

edited

Loading